Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chore: Fix pylint issues with new version of pylint #150

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

addyess
Copy link
Contributor

@addyess addyess commented Sep 20, 2024

Overview

With more linting powers comes more linting issues

Details

@addyess addyess requested a review from a team as a code owner September 20, 2024 16:17
Copy link
Contributor Author

@addyess addyess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotations of changes

Comment on lines -57 to +62
StatusNodeUnavailable: returned when the node isn't in the cluster
StatusNodeInUse: returned when the node is in the cluster already
STATUS_NODE_UNAVAILABLE: returned when the node isn't in the cluster
STATUS_NODE_IN_USE: returned when the node is in the cluster already
"""

StatusNodeUnavailable = 520
StatusNodeInUse = 521
STATUS_NODE_UNAVAILABLE = 520
STATUS_NODE_IN_USE = 521
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pylint wanted const values to be CAPITALIZED

class UserFacingDatastoreConfig(BaseModel, allow_population_by_field_name=True): # type: ignore[call-arg]
class UserFacingDatastoreConfig(BaseModel, allow_population_by_field_name=True):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't in error any longer?
🤷🏻

ops==2.16.0
ops==2.16.1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better typing in ops

logging.info(f"Downloading {source_url}")
logging.info("Downloading %s", source_url)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proper way to use a logger... formatting is done in the logger only if that log level is active

with open(filepath, "w") as f:
with open(filepath, "w", encoding="utf-8") as f:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is a new pylint thing. whenever you open a file for reading/writing specify its encoding.

@@ -49,11 +67,10 @@ class ClusterTokenType(Enum):
NONE = ""


class TokenManager:
class TokenManager(Protocol):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a Protocol isn't a base class, it's a type-definition for duck-typed classes.

charms/worker/k8s/src/token_distributor.py Show resolved Hide resolved
Comment on lines -482 to -484
tokenizer = self.token_strategies.get(token_strategy)
assert tokenizer, f"Invalid token_strategy: {token_strategy}" # nosec

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pylint was fussing about too many local variables. This is a bit of code golf to slim down the variables

@@ -45,21 +45,21 @@ def test_event_timer_properties(harness):


@mock.patch("reschedule._execute_command")
def test_event_timer_is_active(exec, harness):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exec is a keyword in python -- thou shalt not use it for your variables

@@ -45,7 +45,7 @@ plugins = "pydantic.mypy"
[tool.pylint]
# Ignore too-few-public-methods due to pydantic models
# Ignore no-self-argument due to pydantic validators
disable = "wrong-import-order,redefined-outer-name,too-many-instance-attributes,too-few-public-methods,no-self-argument,fixme,parse-error"
disable = "wrong-import-order,redefined-outer-name,too-many-instance-attributes,too-few-public-methods,no-self-argument,fixme,protected-access"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i really want to test some protected member functions of the snap class. So-- lets ignore those errors

Copy link
Member

@mateoflorido mateoflorido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks

@addyess addyess merged commit d04f5f4 into main Sep 20, 2024
34 checks passed
@addyess addyess deleted the chore/linter-update branch September 20, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants